Still need to add pane-group ability as per #160. but we should probably wait until the dropdown is refactored for that.

@ajoslin Do you consider embody #195 or shall I open a new PR when this one is accepted?

Let's discuss it in that issue, I'm not sure about that one.

Sorry for all the re-commits and ci notifications, people :-)

I keep finding little things to add tests for & cases I didn't think of.

More tests are always good. :)

Sent from my iPhone

On Mar 31, 2013, at 9:34 AM, Andy Joslin [email protected] wrote:

Sorry for all the re-commits and ci notifications, people :-)

I keep finding little things to add tests for / cases I didn't think of.

—
Reply to this email directly or view it on GitHub.

@ajoslin I had a quick look and it looks really good, nice and clean. I'm not sure I can catch more by reviewing code. @angular-ui/bootstrap would be awesome to have another look and merge it so this change makes it to a next release.

@ajoslin I would only change a commit message to automatically close all the GH issues. We will also have to communicate clearly to indicate that this is massively breaking change :-)

There is a bug when for example there are nested tabs. I think this has to do with the "Only the active tab's content is actually ever in the DOM" feature, but @ajoslin knows better :-)

It will be nice to have this one into the next release, as it definitely makes the library better.

Yes you're right! I think the tabs always lose their state when you switch. I will have to put it back to the way it used to be.

could we just use the scope delegate '&' on the tabset to provide a callback for tab changes. like

scope {
  postSelect: '&'
}

it seems like this would be a lot cleaner, and avoid the $eval.

Ok, fixed the problem with tabs losing their state, and added tests for it! Thanks @bekos! Was easier than I thought it would be. When you switch tabs, it now just removes the old tab element from the DOM instead of setting the contents' html to an empty string.

@FAQinghere - Doesn't using the '&' binding require the user to define a callback? I didn't think the scope.$eval was that messy, it's a one-liner after all.

@pkozlowski-opensource Perhaps for version 0.3.0 we should add in a tabs directive which simply throws an error, giving a link to an explanation of how to migrate to the new directive? We can take it out in 0.4.0.

Something like:

angular.module('ui.bootstrap.tabs').directive('tabs', function() {
  return function() {
    throw new Error("The `tabs` directive is deprecated, please migrate to `tabset`. Instructions can be found at http://github.com/angular-ui/bootstrap/tree/master/CHANGELOG.md#0.3.0-tabs");
  };
});

OK, implemented all of Pete's changes, as well as some general documentation polish.

Look good to you, Pete?

Updated the commit message with a BREAKING CHANGE section in the footer, according to the Git Commit Message Conventions doc :-)

I love the functional changes, in general, but I think (I'll give it a little time today or tomorrow) that the implementation could be significantly simpler.

@FAQinghere if you can figure out how to make tab-heading a directive, do it! It'd be better. The problem was by the time the tab-heading directive got compiled, it was already transcluded into the content area and had no idea what tab it was part of. I tried a lot of things...

@petebacondarwin Good point, that was old. removed that :-)

Sorry, I got pulled into another project and don't have the time to follow up on this right now, but I'll make a few notes.

I think it would be better to make the mark up more explicit. It would make it easier to clean up the implementation, and also make it more parallel to standard markup.

example:

<tabset>
  <tab>
    <tabhead></tabhead>
    <tabpane></tabpane>
  </tab>
</tabset>

We could use the delegating scope variable '&' to provide a simple callback to the outer scope if desired (using an attribute such as on-nav:"changedTab(tabIndex, tabName)".

We could use restrict: "EA" on all the directives. When we need to capture the inner content (on tabhead, because it actually belongs outside the tab, on the tabset, in terms of final markup), we can use 'replace' internally to restrict it to the tag form (might need a separate tag name internally because Angular doesn't recognize that the tag is being replaced and will think it's getting redundant copies of the tag and throw an error). Then we can easily pull the inner content from the tabhead using el.find('customtag'), avoiding the need to process both tags and attributes, even though the original markup can make use of both.

We could eliminate 'heading' attribute altogether, as users could use the tabhead directive. On the other hand, we might add in a name attribute on the tag, which could be passed to the outer callback if provided, as mentioned above.

Looking at the current implementation, I think this could add functionality and decrease the code considerably.

I think the thing is that for many cases people don't want complex headings
and that the simple heading attribute and content situation is cleaner.

On 10 April 2013 15:13, FAQinghere [email protected] wrote:

Sorry, I got pulled into another project and don't have the time to follow
up on this right now, but I'll make a few notes.

I think it would be better to make the mark up more explicit. It would
make it easier to clean up the implementation, and also make it more
parallel to standard markup.

example:

We could use the delegating scope variable '&' to provide a simple
callback to the outer scope if desired (using an attribute such as
on-nav:"changedTab(tabIndex, tabName)".

We could use restrict: "EA" on all the directives. When we need to capture
the inner content (on tabhead, because it actually belongs outside the tab,
on the tabset, in terms of final markup), we can use 'replace' internally
to restrict it to the tag form (might need a separate tag name internally
because Angular doesn't recognize that the tag is being replaced and will
think it's getting redundant copies of the tag and throw an error). Then we
can easily pull the inner content from the tabhead using
el.find('customtag'), avoiding the need to process both tags and
attributes, even though the original markup can make use of both.

We could eliminate 'heading' attribute altogether, as users could use the
tabhead directive. On the other hand, we might add in a name attribute on
the tag, which could be passed to the outer callback if provided, as
mentioned above.

Looking at the current implementation, I think this could add
functionality and decrease the code considerably.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/287#issuecomment-16176472
.

I thought about that too. In that case, we could use the name (or 'heading', etc) attribute to build the tabhead in cases where one isn't given. Either way the dom has to be manipulated manually for the tabhead because it belongs at a higher level in the final html. That still leaves the 'tab-pane' directive open to debate of course. On the other hand, if Angular's goal is to teach HTML new tricks, it seems to make sense that the markup used should be as HTML-like as possible. A tiny bit of additional markup isn't evil if it makes the underlying implementation cleaner and more robust.

@FAQinghere we are trying to avoid super verbose markup like that - although it would fix some of the problems, we don't want to return to the ultra-verbosity of twitter bootstrap's own js/markup :-)

@angular-ui/bootstrap do you guys think this is this good to merge?

@ajoslin Thanks a lot for your work on this refactor.
I've already started using it in a project (with <tab-heading>). Unfortunately I just found out that it's broken :
If you open a tab that's already been visited once, all controls are dead in the tab content.

Also, it looks like the <tabset> element isn't replaced by a <div>; it's left as-is in the output HTML.

http://plnkr.co/edit/yWfhKJ?p=preview

Thanks @Nandan108 - I'll look into this. I may just give up on having only the active tab's element in the DOM :-)

Just a note - I am working on this but have had a bit of busyness lately (moving). Tonight or early tomorrow I'll have the refactor done - just gotta finish changing the unit tests. Hopefully we can get it pushed with 0.3.0 soon 😄

Awesome @ajoslin And don't stress about it :-)

OK, fixed tabs to ng-repeat all the tabs and display only the active ones now! Also tests are more awesome, and I rebased it onto master.

Seems like this is all working. At least, it is for me.
So are the new tabs getting merged into 0.3.0, or is that too late already?

It's too late for that :-)

Let's merge it into master though! Everyone good with that @angular-ui/bootstrap ?

I wanted to merge it yesterday alongside with other PRs but for some reason CI server is stuck on git pooling:
http://ci.angularjs.org/job/angularui-bootstrap/scmPollLog/?

Guess it died so we can't run tests on all the browsers :-(

Weird! Have you e-mailed Igor? I will and cc everyone if not.

@ajoslin nope, I didn't sorry, trying to push next chapter of the book today, totally forgotten :-(

I saw it handing previously and saw some info on the net regarding Jenkins + git problems. We should probably ask Igor to configure a hook so the build is started by push to GitHub and not based on pooling. I think it is set up like this for the AngularJS project.

https://wiki.jenkins-ci.org/display/JENKINS/GitHub+Plugin

+1 :)

Desperate for this. When do you think we'll get it? Tks for the hard work.

@angular-ui/bootstrap ready to merge this? :-) CI issues are fixed. I will squash and rebase against master and push again to be sure.

@ajoslin let's merge it! I haven't had time to review it but I'm sure that you did great job. And we can always fix things is something goes wrong!

Landed in c532659 :-)

So... how is the "select" attribute going? do we have this feature on master? or did anyone get a workarround to get notified on tab selection?

Sorry i did not know the proper place to ask this :_)

This is already on master!

Glad to hear it... pane select="callback" ?

Mmmm, the new tabset tab structure doesn't seem to work for me, i downloaded a snapshot of 0.4... should i clone master and build the js myself? how do i build it?

Sorry, i know those are noob questions... but that's what i am right now :_)

Download from github is not enough, you have to use grunt to build the files.

Grunt, ok, thank you man!

Is this part of 0.3.0? I downloaded the build and it looks like it still has the old tabs and panes directives.

@chiester nope, it will be only released as part of 0.4.0

Can I build it with the new tabs component?

@chiester just build the latest master, it should be pretty stable.

I was specifically looking for the new feature in tabs where it allows a callback on select. Thanks.